Skip to content

Conversation

@cong-or
Copy link
Contributor

@cong-or cong-or commented Oct 22, 2024

@cong-or cong-or self-assigned this Oct 22, 2024
@cong-or cong-or changed the title feat(rust/immutable ledger): block ser/de and validation feat(rust/immutable-ledger): block ser/de and validation Oct 22, 2024
@cong-or cong-or changed the title feat(rust/immutable-ledger): block ser/de and validation feat(rust/immutable-ledger): block encoding decoding and validation Oct 22, 2024
@cong-or
Copy link
Contributor Author

cong-or commented Oct 30, 2024

I think from the API surface we should have something like this

struct BlockHeader {
   chain_id: ULID,
    height: u32,
   ...
}

impl BlockHeader {
   
   pub fn to_bytes(&self) -> anyhow::Result<Vec<u8>> {
    ...
   }
   pub fn from_bytes(bytes: &[u8]) -> anyhow::Result<Self> {
    ...
   }
}

And the same for other types like Signatures, Block etc.

Also I think we should not split the definition from the structural perspective a common block from the genesis or final. They all blocks with the same structure. So I am proposing to add a methods like

impl Block {
  pub fn is_genesis(&self) -> bool {}
  
  pub fn is_final(&self) -> bool {}
}

And leave all the validation logic under the validate(&self) method, and not do any validation during decoding.

What do you think @cong-or ?

ok yes, I will encapsulate the logic in types as suggested. I assumed the ledger was going to be written as a contiguous block of memory hence thinking purely in bytes rather than types.

@Mr-Leshiy
Copy link
Contributor

@cong-or

I assumed the ledger was going to be written as a contiguous block of memory hence thinking purely in bytes rather than types.

Yea, you are right, from the storage perspective it will be how you described.

BTW, I am going to move this immutable_ledger documentation to this repo, so we will have a direct access to the block.cddl file.
What do you think to have as a part of the from_bytes decoding logic to have a strict validation agains the cddl spec. The same as we did with the cip36 registration data ?
Or maybe just validate it in unit tests, but seems to me we need to have a strict check somewhere that this decoding logic conforms to the original design doc.

@Mr-Leshiy
Copy link
Contributor

@cong-or just created a PR #72

Also forget to mention, from the testing perspective I think it make sense to make such decoding tests as property tests,
e.g.

fn encrypted_vote_to_bytes_from_bytes_test(

@cong-or cong-or closed this Nov 1, 2024
@cong-or cong-or reopened this Nov 3, 2024
Copy link
Contributor

@Mr-Leshiy Mr-Leshiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for now you can totally remove cddl validation.
We definitely need such validation, but will try to add it later.

@cong-or cong-or enabled auto-merge (squash) November 14, 2024 10:23
@minikin minikin self-requested a review November 14, 2024 12:42
@cong-or cong-or merged commit 405f07a into main Nov 14, 2024
24 checks passed
@cong-or cong-or deleted the block_ser_ledger branch November 14, 2024 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review me PR is ready for review

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants